Remove separate indexing of early-bound regions #99821

cjgillot · 2022-07-27T20:25:41Z

~~Based on #99728.~~

This PR copies some modifications from #97839 around object lifetime defaults.
These modifications allow to stop counting generic parameters during lifetime resolution, and rely on the indexing given by rustc_typeck::collect.

rust-highfive · 2022-07-27T20:25:45Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot · 2022-07-27T20:25:45Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

bors · 2022-08-02T20:50:17Z

☔ The latest upstream changes (presumably #100063) made this pull request unmergeable. Please resolve the merge conflicts.

estebank · 2022-08-04T13:42:12Z

r? rust-lang/compiler

nagisa · 2022-08-15T19:24:27Z

r? rust-lang/compiler

I’ll be out on vacation for a while, so I won’t be able to get to this anytime soon.

nagisa · 2022-08-15T19:25:05Z

(well, highfive should be adjusted somehow to not pass the buck straight back to the original reviewer ^^)

estebank · 2022-08-19T14:01:20Z

I'm not confident reviewing this PR right now. Let's see if rerolling yields comedy.

r? rust-lang/compiler

wesleywiser · 2022-08-23T21:55:29Z

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -1044,6 +1044,11 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
                    record_array!(self.tables.inferred_outlives_of[def_id] <- inferred_outlives);
                }
            }
+            if let DefKind::TyParam | DefKind::ConstParam = def_kind {


I see other places in this PR were updated to handle DefKind::LifetimeParam | DefKind::TyParam | DefKind::ConstParam, should this also check for DefKind::LifetimeParam?

wesleywiser · 2022-08-23T22:08:44Z

This looks good to me but I'm not an expert in this part of the compiler.

r? rust-lang/types

compiler-errors · 2022-08-29T02:13:54Z

@bors r+

bors · 2022-08-29T02:13:56Z

📌 Commit da90ec1 has been approved by compiler-errors

It is now in the queue for this repository.

…r-errors Remove separate indexing of early-bound regions ~Based on rust-lang#99728 This PR copies some modifications from rust-lang#97839 around object lifetime defaults. These modifications allow to stop counting generic parameters during lifetime resolution, and rely on the indexing given by `rustc_typeck::collect`.

Rollup of 8 pull requests Successful merges: - rust-lang#98304 (Add MaybeUninit memset test) - rust-lang#98801 (Add a `File::create_new` constructor) - rust-lang#99821 (Remove separate indexing of early-bound regions) - rust-lang#100239 (remove an ineffective check in const_prop) - rust-lang#100337 (Stabilize `std::io::read_to_string`) - rust-lang#100819 (Make use of `[wrapping_]byte_{add,sub}`) - rust-lang#100934 (Remove a panicking branch from `fmt::builders::PadAdapter`) - rust-lang#101000 (Separate CountIsStar from CountIsParam in rustc_parse_format.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

nnethercote · 2022-08-30T03:04:06Z

@cjgillot: the perf results here show that this was overall a performance win, but there were some regressions for diesel-1.4.8. Any idea what might be the cause, and how to remedy?

nnethercote · 2022-08-30T03:56:06Z

Here is the first part of the Cachegrind diff, if that's helpful.

--------------------------------------------------------------------------------
Ir
--------------------------------------------------------------------------------
46,187,526  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir          file:function
--------------------------------------------------------------------------------
 8,016,144  ???:rustc_resolve::late::lifetimes::object_lifetime_default
 7,919,529  ???:<rustc_metadata::rmeta::encoder::EncodeContext>::encode_def_ids
 7,751,226  ???:rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::plumbing::QueryCtxt, rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, core::option::Option<rustc_middle::middle::resolve_lifetime::ObjectLifetimeDefault>>>
-6,925,800  ???:<rustc_middle::ty::generics::Generics as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
 6,027,485  ???:<rustc_hir::hir::WhereBoundPredicate>::is_param_bound
 4,748,382  ???:<alloc::rc::Rc<alloc::vec::Vec<rustc_ast::tokenstream::TokenTree>> as core::ops::drop::Drop>::drop
 2,937,328  ???:<rustc_data_structures::sip128::SipHasher128>::finish128
-2,538,406  ???:<alloc::vec::Vec<rustc_ast::tokenstream::TokenTree> as core::ops::drop::Drop>::drop
-2,508,142  ???:rustc_middle::ty::codec::encode_with_shorthand::<rustc_query_impl::on_disk_cache::CacheEncoder, rustc_middle::ty::Ty, <rustc_query_impl::on_disk_cache::CacheEncoder as rustc_type_ir::codec::TyEncoder>::type_shorthands>
 2,311,692  ???:<hashbrown::raw::RawTable<(rustc_span::def_id::DefId, (core::option::Option<rustc_hir::def::DefKind>, rustc_query_system::dep_graph::graph::DepNodeIndex))>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_span::def_id::DefId, rustc_span::def_id::DefId, (core::option::Option<rustc_hir::def::DefKind>, rustc_query_system::dep_graph::graph::DepNodeIndex), core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
-2,311,692  ???:<hashbrown::raw::RawTable<(rustc_middle::ty::Ty, usize)>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_middle::ty::Ty, rustc_middle::ty::Ty, usize, core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
 2,207,430  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
 1,747,207  ???:<std::collections::hash::map::HashMap<rustc_hir::hir_id::ItemLocalId, &rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg>, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 1,621,651  ???:<rustc_span::symbol::Symbol as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
 1,335,627  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::is_green
 1,320,616  ???:<rustc_middle::dep_graph::dep_node::DepKind as rustc_query_system::dep_graph::DepKind>::read_deps::<<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::read_index::{closure
 1,277,536  ???:rustc_query_system::query::plumbing::incremental_verify_ich::<rustc_query_impl::plumbing::QueryCtxt, rustc_span::def_id::DefId, core::option::Option<rustc_middle::middle::resolve_lifetime::ObjectLifetimeDefault>>
 1,232,078  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::prev_fingerprint_of
 1,225,744  ???:<rustc_middle::hir::map::Map>::get_generics

cjgillot · 2022-08-31T21:44:54Z

The main difference I see between diesel, which regresses, and all the other cases is that diesel uses many more trait objects. As this PR changes how object lifetime defaults are computed, having an effect correlated to the presence of trait object is consistent. That said, it does not explain the difference.
The detailed time view would point towards the object_lifetime_default query overhead and metadata generation cost.
I tried #101214 but this does not seem to help.

rust-highfive assigned estebank Jul 27, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2022

This comment has been minimized.

Sign in to view

cjgillot force-pushed the ast-lifetimes-2 branch from 96c1f7a to 2eafc90 Compare July 27, 2022 21:40

cjgillot added 9 commits August 3, 2022 18:32

Make object_lifetime_defaults a cross-crate query.

39bc74e

Create a specific ObjectLifetimeDefault enum.

236ccce

Compute object_lifetime_default per parameter.

99e2d33

Create lifetime parameter like any other parameter.

63c3aab

Remove index from Region::EarlyBound.

421bb6a

Use DefIdTree instead of open-coding it.

48bae93

Simplify debugging.

a424090

Assert index sanity.

c95ff1d

Bless incremental tests.

da90ec1

cjgillot force-pushed the ast-lifetimes-2 branch from 2eafc90 to da90ec1 Compare August 3, 2022 16:44

rust-highfive assigned nagisa and unassigned estebank Aug 4, 2022

rust-highfive assigned estebank and unassigned nagisa Aug 15, 2022

rust-highfive assigned wesleywiser and unassigned estebank Aug 19, 2022

wesleywiser reviewed Aug 23, 2022

View reviewed changes

rust-highfive assigned compiler-errors and unassigned wesleywiser Aug 23, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2022

Dylan-DPC mentioned this pull request Aug 29, 2022

Rollup of 7 pull requests #101148

Closed

Dylan-DPC mentioned this pull request Aug 29, 2022

Rollup of 8 pull requests #101152

Merged

bors merged commit 5555e13 into rust-lang:master Aug 29, 2022

rustbot added this to the 1.65.0 milestone Aug 29, 2022

cjgillot deleted the ast-lifetimes-2 branch August 29, 2022 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove separate indexing of early-bound regions #99821

Remove separate indexing of early-bound regions #99821

cjgillot commented Jul 27, 2022 •

edited

Loading

rust-highfive commented Jul 27, 2022

rustbot commented Jul 27, 2022

This comment has been minimized.

bors commented Aug 2, 2022

estebank commented Aug 4, 2022

nagisa commented Aug 15, 2022

nagisa commented Aug 15, 2022

estebank commented Aug 19, 2022

wesleywiser Aug 23, 2022

wesleywiser commented Aug 23, 2022

compiler-errors commented Aug 29, 2022

bors commented Aug 29, 2022

nnethercote commented Aug 30, 2022

nnethercote commented Aug 30, 2022

cjgillot commented Aug 31, 2022

Remove separate indexing of early-bound regions #99821

Remove separate indexing of early-bound regions #99821

Conversation

cjgillot commented Jul 27, 2022 • edited Loading

rust-highfive commented Jul 27, 2022

rustbot commented Jul 27, 2022

This comment has been minimized.

bors commented Aug 2, 2022

estebank commented Aug 4, 2022

nagisa commented Aug 15, 2022

nagisa commented Aug 15, 2022

estebank commented Aug 19, 2022

wesleywiser Aug 23, 2022

Choose a reason for hiding this comment

wesleywiser commented Aug 23, 2022

compiler-errors commented Aug 29, 2022

bors commented Aug 29, 2022

nnethercote commented Aug 30, 2022

nnethercote commented Aug 30, 2022

cjgillot commented Aug 31, 2022

cjgillot commented Jul 27, 2022 •

edited

Loading